Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor InternalUsers REST API test #4481

Merged
merged 1 commit into from
Jul 5, 2024
Merged

Refactor InternalUsers REST API test #4481

merged 1 commit into from
Jul 5, 2024

Conversation

willyborankin
Copy link
Collaborator

@willyborankin willyborankin commented Jun 21, 2024 •
edited
Loading

Description

Split up UserApiTest into 3 diff tests:

  • InternalUsersRestApiIntegrationTest
  • InternalUsersRegExpPasswordRulesRestApiIntegrationTest
  • InternalUsersScoreBasedPasswordRulesRestApiIntegrationTest

Fixes:

Check List

  • New functionality includes testing
  • New functionality has been documented
  • New Roles/Permissions have a corresponding security dashboards plugin PR
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@codecov Codecov
Copy link

codecov bot commented Jun 21, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.

Project coverage is 65.00%. Comparing base (2b5a811) to head (7552b0f).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4481      +/-   ##
==========================================
- Coverage   65.17%   65.00%   -0.18%     
==========================================
  Files         313      313              
  Lines       22053    22064      +11     
  Branches     3561     3562       +1     
==========================================
- Hits        14374    14342      -32     
- Misses       5904     5941      +37     
- Partials     1775     1781       +6
Files Coverage Δ
...security/dlic/rest/api/InternalUsersApiAction.java 76.85% <100.00%> (-4.72%) ⬇️
...java/org/opensearch/security/user/UserService.java 45.32% <80.00%> (-11.72%) ⬇️

... and 9 files with indirect coverage changes

shikharj05
shikharj05 suggested changes Jun 24, 2024
View reviewed changes
Copy link
Contributor

@shikharj05 shikharj05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @willyborankin for the changes, tests look much better now! Code changes look good to me, minor comment around adding comments in tests.

...java/org/opensearch/security/api/InternalUsersRegExpPasswordRulesRestApiIntegrationTest.java Show resolved Hide resolved
@willyborankin willyborankin added the backport 2.x backport to 2.x branch label Jun 24, 2024
@willyborankin willyborankin force-pushed the refactor-tests#9 branch 3 times, most recently from f2ef3d2 to 7beb272 Compare June 26, 2024 19:45
@cwperks
Copy link
Member

cwperks commented Jul 1, 2024

@codecov/project/plugin — 64.66% (-0.29%) compared to 6dedfb4

@willyborankin Are there additional use-cases that need to be included to ensure the coverage is not decreased with migrating the tests to integrationTest framework?

@willyborankin
Copy link
Collaborator Author

@codecov/project/plugin — 64.66% (-0.29%) compared to 6dedfb4

@willyborankin Are there additional use-cases that need to be included to ensure the coverage is not decreased with migrating the tests to integrationTest framework?

Good point, I did not revert change which I made. Hope it will be better

@willyborankin
Copy link
Collaborator Author

@codecov/project/plugin — 64.66% (-0.29%) compared to 6dedfb4

@willyborankin Are there additional use-cases that need to be included to ensure the coverage is not decreased with migrating the tests to integrationTest framework?

@cwperks the different in the test coverage is due to UserService in case of service users all other looks ok.

@willyborankin
Refactor InternalUsers REST API test
7552b0f 
Split up UserApiTest into 3 diff tests:
 - InternalUsersRestApiIntegrationTest
 - InternalUsersRegExpPasswordRulesRestApiIntegrationTest
 - InternalUsersScoreBasedPasswordRulesRestApiIntegrationTest

 Fixes:
 - Added validation for the security roles for PATCH
 - Added validation for restricted character in username for PATCH

Signed-off-by: Andrey Pleskach <ples@aiven.io>
shikharj05
shikharj05 approved these changes Jul 5, 2024
View reviewed changes
Copy link
Contributor

@shikharj05 shikharj05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes! @willyborankin

@willyborankin willyborankin merged commit a588bdd into opensearch-project:main Jul 5, 2024
40 of 42 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.x
# Create a new branch
git switch --create backport/backport-4481-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 a588bdd4f09aff813ef0ebfce051246740680ae6
# Push it to GitHub
git push --set-upstream origin backport/backport-4481-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-4481-to-2.x.

willyborankin added a commit to willyborankin/security that referenced this pull request Jul 5, 2024
@willyborankin
Refactor InternalUsers REST API test (opensearch-project#4481)
be3fa78 
Signed-off-by: Andrey Pleskach <ples@aiven.io>
(cherry picked from commit a588bdd)
@willyborankin willyborankin mentioned this pull request Jul 5, 2024
Merged
willyborankin added a commit to willyborankin/security that referenced this pull request Jul 5, 2024
@willyborankin
Refactor InternalUsers REST API test (opensearch-project#4481)
4e5332c 
Signed-off-by: Andrey Pleskach <ples@aiven.io>
(cherry picked from commit a588bdd)
willyborankin added a commit that referenced this pull request Jul 8, 2024
@willyborankin
[Backport 2.x] Refactor InternalUsers REST API test (#4481) (#4527)
47effae
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shikharj05 shikharj05 shikharj05 approved these changes

@cwperks cwperks cwperks approved these changes

@DarshitChanpura DarshitChanpura DarshitChanpura approved these changes

@cliu123 cliu123 Awaiting requested review from cliu123

@peternied peternied Awaiting requested review from peternied peternied is a code owner

@RyanL1997 RyanL1997 Awaiting requested review from RyanL1997 RyanL1997 is a code owner

@stephen-crawford stephen-crawford Awaiting requested review from stephen-crawford

@reta reta Awaiting requested review from reta reta is a code owner

Assignees
No one assigned
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@willyborankin @cwperks @shikharj05 @DarshitChanpura